Deprecate snap server experimental option and add stable option #8512
Conversation
macfarla
added
the
doc-change-required
alexandratran
removed
the
doc-change-required
|
@garyschulte are we ready to remove the --X from snap server? |
besu/src/main/java/org/hyperledger/besu/cli/options/SynchronizerOptions.java
Show resolved
Hide resolved
| @@ -445,7 +457,7 @@ public SynchronizerConfiguration.Builder toDomainObject() { | |||
| .trienodeCountPerRequest(snapsyncTrieNodeCountPerRequest) | |||
| .localFlatAccountCountToHealPerRequest(snapsyncFlatAccountHealedCountPerRequest) | |||
| .localFlatStorageCountToHealPerRequest(snapsyncFlatStorageHealedCountPerRequest) | |||
| .isSnapServerEnabled(snapsyncServerEnabled) | |||
| .isSnapServerEnabled(snapsyncServerEnabled || snapsyncServerEnabledExperimental) | |||
There was a problem hiding this comment.
This isn't the flow we typically use when we promote something out of experimental. Typically we just alias the -X and after a quarter or so just remove the flag. We can add a deprecation message without adding an additional flag .
That said, this isn't a horrible pattern. At a minimum there needs to be a CHANGELOG.md entry for this PR, and some text in the upcoming deprecation body that references it.
pullurib
force-pushed
the
snapserver-option
branch
from
6db1bf9 to
f53300f
Compare
Signed-off-by: Bhanu Pulluri <[email protected]>
Signed-off-by: Bhanu Pulluri <[email protected]>
Signed-off-by: Bhanu Pulluri <[email protected]>
pullurib
force-pushed
the
snapserver-option
branch
from
154f4ce to
048df22
Compare
Signed-off-by: Bhanu Pulluri <[email protected]>
Signed-off-by: Bhanu Pulluri <[email protected]>
Signed-off-by: Bhanu Pulluri <[email protected]>
Signed-off-by: Bhanu Pulluri <[email protected]>
Signed-off-by: Bhanu Pulluri <[email protected]>
Signed-off-by: Bhanu Pulluri <[email protected]>
| @@ -297,11 +300,13 @@ public void parseBlockPropagationRange(final String arg) { | |||
| SnapSyncConfiguration.DEFAULT_LOCAL_FLAT_STORAGE_COUNT_TO_HEAL_PER_REQUEST; | |||
|
|
|||
| @CommandLine.Option( | |||
| names = SNAP_SERVER_ENABLED_FLAG, | |||
| names = {SNAP_SERVER_ENABLED_FLAG, SNAP_SERVER_ENABLED_EXPERIMENTAL_FLAG}, | |||
| hidden = true, | |||
There was a problem hiding this comment.
need to unhide this option. check that it shows up in besu -h
There was a problem hiding this comment.
@pullurib apologies, I didn't notice this before - once this is fixed I'm happy!
There was a problem hiding this comment.
No worries @macfarla , but running into this error :
warning: [ExperimentalCliOptionMustBeCorrectlyDisplayed] Experimental options must be hidden and not present in the BesuCommand class.
I guess it can be unhidden when the experimental option is removed in future. Or need to have two separate options as I had earlier but looks like that's not a pattern used for deprecating experimental options.
There was a problem hiding this comment.
ahhhh I see. maybe that is a reason to have the two separate options 🙈
I think it would be weird to announce production ready feature but it doesn't show in the help. @garyschulte @jflo wdyt
There was a problem hiding this comment.
yes, agree both options should be included while the experimental variant is deprecated. We can remove the -X variant in the next release.
There was a problem hiding this comment.
@pullurib you can use @SuppressWarnings("ExperimentalCliOptionMustBeCorrectlyDisplayed")
you'll find examples in the codebase
There was a problem hiding this comment.
then you don't need to have 2 separate options
PR description
Deprecate --Xsnapsync-server-enabled and add --snapsync-server-enabled instead.
Fixed Issue(s)
Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew spotlessApply./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests